Add separate checkout info block to buildscripts. #3483

mitchell-as · 2024-09-09T18:56:59Z

DX-3024 Non-versioned buildscript information is clearly denoted

Use triple-backticks to separate checkout info from buildscript. Using triple-quotes will require a new parser and/or lexer. Since we're experimenting, let's use what we have.
Buildscripts always have an AtTime (it's no longer optional). I believe we had it optional for compatiblity with build expressions that didn't have a separate commit time, but all build expressions now have an associated commit time from what I can tell.
Renamed localcommit package to checkoutinfo
- checkoutinfo is a primer property and all runners go through it to get commit IDs and work with build script (initialize, update, etc.).
- Removed runbits/buildscript/file.go in favor of this package.
Changed buildplanner.GetBuildScript() to additionally accept an owner, project, branch, and commitID. It uses these fields to construct the "Project" field in the commit info section of buildscripts.
state reset LOCAL will always re-initialize a build script.
Internal change in the raw buildscript format to not store strings with quotes. (Unmarshaling strips quotes, and marshaling adds them back.)
Users are shown a warning if they attempt to use an outdate build script (state install, etc.).

Of the form: ``` Project: URL Time: time ```

To do this, we need callers to pass in the project namespace.

Also rename Get and Set methods to be for commit IDs.

mitchell-as · 2024-09-09T20:43:49Z

pkg/projectfile/projectfile.go

+		pjf := NewProjectField()
+		pjf.LoadProject("https://" + host)
+		pjf.SetNamespace(params.Owner, params.Project)
 		if params.BranchName != "" {
-			q.Set("branch", params.BranchName)
+			pjf.SetBranch(params.BranchName)
 		}
-
-		u.RawQuery = q.Encode()
-		params.ProjectURL = u.String()
+		params.ProjectURL = pjf.String()


This is to avoid code duplication. I was using NewProjectField() in the buildplanner to construct the "Project" field in the checkout info block of buildscripts, and found that we can do the same thing here.

mitchell-as · 2024-09-10T15:56:55Z

Test failure is unrelated to this PR.

This reverts commit e036299.

Naatan

Please see comment on the Initialize function. I didn't finish the review as I imagine this comment may bring about some significant changes.

internal/runbits/buildscript/file.go

We need to keep them separate.

internal/runners/reset/reset.go

pkg/buildscript/unmarshal.go

pkg/buildscript/unmarshal_buildexpression.go

pkg/checkoutinfo/checkoutinfo.go

Call out to build scripts to get and set commit IDs instead of using another package. The build script packages will determine if build scripts are enabled and use a mediator if necessary.

mitchell-as · 2024-09-12T21:43:09Z

I expect more churn. I'm flagging for early review as I doubt it'll pass all tests in its present state, but the idea is in place.

Naatan

Two things stand out to me at a high level:

I think checkoutinfo should probably be a primer property
- Because the reliance on cfg already implies primer anyway, and similar to project/projectfile this will almost always be relevant for every command.
We're still sourcing project URL info from the project package
- eg. owner, name and branch are still being sourced from project. I'd expect to see these sourced from checkoutinfo, and have legacy methods on the project methods like we did with the commitID. These legacy methods should probably only be called from checkoutinfo, but there might be edge cases.

Naatan · 2024-09-13T17:17:47Z

internal/runbits/buildscript/commitid.go

+	"github.com/ActiveState/cli/pkg/checkoutinfo"
+)
+
+func CommitID(path string, cfg configurer) (strfmt.UUID, error) {


Can we move this to checkoutinfo? I get where you're coming from, but runbits are about sharing controller logic. This isn't controller logic. And this type of muliplexing is exactly what the checkoutinfo package is intended for.

… initialize and update build scripts.

…ldScript()` method.

mitchell-as · 2024-09-16T17:42:34Z

I made checkout info a primer property and I think I addressed your other line item (sorry if I overlooked it; lots of changes).

I've updated the PR description with an overview of the entire thing.

Naatan · 2024-09-19T16:22:56Z

pkg/checkoutinfo/checkoutinfo.go

+		} else if !errors.Is(err, ErrBuildscriptNotExist) {
+			return "", errs.Wrap(err, "Could not get build script")
+		}
+		// Fall back on activestate.yaml.


If the buildscript exists then I think we should just fail here. Falling back to the as.yaml at that point is working around some bug.

Naatan · 2024-09-19T16:24:44Z

pkg/checkoutinfo/checkoutinfo.go

+	return c.project.Owner()
+}
+
+func (c *CheckoutInfo) Name() string {
+	return c.project.Name()
+}
+
+func (c *CheckoutInfo) Branch() string {
+	return c.project.BranchName()


These should behave the same as the CommitID() function; ie. collect from buildscript if opted in.

Also their Set() methods should be in checkoutinfo also.. otherwise we're not syncing the buildscript when those are changed.

I'm going to remove the Owner(), Name(), and Branch() accessors since they're not used externally. The ACs make no mention of having checkoutinfo being the gatekeeper of accessors, just for setters like you mentioned here.

If we make checkoutinfo the sole source of truth for Owner, Name, and Branch, then this PR will double in size. I'd rather do that in another ticket.

Naatan · 2024-09-19T16:29:19Z

pkg/checkoutinfo/checkoutinfo.go

+	if !fileutils.FileExists(buildscriptPath) {
+		return c.InitializeBuildScript(commitID)
+	}


Please error out with the "run state reset LOCAL" error. This file should exist if opted in, it not existing is an error condition. Working around this magically like this is what we did originally and it caused a lot of issues.

Naatan · 2024-09-19T16:34:16Z

pkg/checkoutinfo/checkoutinfo.go

+	return strfmt.UUID(commitID), nil
+}
+
+func (c *CheckoutInfo) BuildScript() (*buildscript.BuildScript, error) {


I'm not sure about having this, UpdateBuildScript(), and InitBuildScript() inside checkoutinfo. Checkoutinfo is from a consumer point of view about project information, not about buildscripts, that's just an under the hood mechanic.

Also, we don't handle project retrieval or initialization in this package, so handling buildscripts here feels inconsistent.

Let's put it this way: if we KNOW we want a buildscript, or a project/projectile (the pkg) then we should not be using checkoutinfo.

mitchell-as · 2024-09-26T18:12:44Z

This is a mess. I'm going to restart.

mitchell-as · 2024-09-26T18:28:20Z

Restarting is more of a mess, so this'll have to do.

mitchell-as · 2024-09-26T19:13:15Z

No, I can't do it in this PR. I'll start from scratch.

mitchell-as added 6 commits September 6, 2024 15:09

Participle should unquote strings.

2081db6

Initial support for commit info in buildscript.

5033503

Of the form: ``` Project: URL Time: time ```

Fetching buildscripts should also populate the Project commit info.

72cb762

To do this, we need callers to pass in the project namespace.

localcommit package should update buildscript's Project commit info.

ec39083

Rename "commit info" to "checkout info" in recent changes.

5b403b6

Rename localcommit package to checkoutinfo.

41ee7a1

Also rename Get and Set methods to be for commit IDs.

mitchell-as added Test: critical Test: buildscripts labels Sep 9, 2024

Fixed failing unit tests.

e504147

mitchell-as force-pushed the mitchell/dx-3024 branch from 5a1ceb5 to e504147 Compare September 9, 2024 19:10

mitchell-as commented Sep 9, 2024

View reviewed changes

Add warning about using an outdated buildscript.

a2e043d

mitchell-as requested a review from Naatan September 10, 2024 16:03

mitchell-as marked this pull request as ready for review September 10, 2024 16:03

mitchell-as added 2 commits September 10, 2024 14:01

Sort artifact changesets by name.

e036299

Revert "Sort artifact changesets by name."

1bbf6c2

This reverts commit e036299.

mitchell-as closed this Sep 10, 2024

mitchell-as reopened this Sep 10, 2024

Naatan requested changes Sep 10, 2024

View reviewed changes

internal/runbits/buildscript/file.go Outdated Show resolved Hide resolved

Pass project details to the buildscript runbit, not project objects.

008161f

We need to keep them separate.

mitchell-as requested a review from Naatan September 10, 2024 21:25

Naatan requested changes Sep 11, 2024

View reviewed changes

mitchell-as added 3 commits September 11, 2024 17:03

Include branch in buildscript commit info.

4a91dda

Use YAML parser when unmarshaling buildscript checkout info.

ccd9554

Update activestate.yaml with any buildscript checkout info changes.

b1bb954

mitchell-as force-pushed the mitchell/dx-3024 branch from 4259078 to b1bb954 Compare September 11, 2024 21:29

Make build scripts first-class citizens.

9bcce03

Call out to build scripts to get and set commit IDs instead of using another package. The build script packages will determine if build scripts are enabled and use a mediator if necessary.

mitchell-as requested a review from Naatan September 12, 2024 21:43

Naatan requested changes Sep 13, 2024

View reviewed changes

Fix some failing tests.

a54541d

mitchell-as force-pushed the mitchell/dx-3024 branch 3 times, most recently from b3810dc to a8c8a35 Compare September 16, 2024 16:22

Make checkoutinfo a primer property and use it to set commit IDs, and…

6b174f8

… initialize and update build scripts.

mitchell-as force-pushed the mitchell/dx-3024 branch from a8c8a35 to 6b174f8 Compare September 16, 2024 16:23

Moved buildscript_runbit.ScriptFromProject() into `checkoutinfo.Bui…

4a150c0

…ldScript()` method.

mitchell-as requested a review from Naatan September 16, 2024 17:42

Naatan requested changes Sep 19, 2024

View reviewed changes

mitchell-as added 2 commits September 25, 2024 16:23

Merge

6a4528c

Cleanup after merge.

796a91b

mitchell-as closed this Sep 26, 2024

mitchell-as reopened this Sep 26, 2024

mitchell-as closed this Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate checkout info block to buildscripts. #3483

Add separate checkout info block to buildscripts. #3483

mitchell-as commented Sep 9, 2024 •

edited by github-actions bot

Loading

mitchell-as Sep 9, 2024

mitchell-as commented Sep 10, 2024

Naatan left a comment

mitchell-as commented Sep 12, 2024 •

edited

Loading

Naatan left a comment

Naatan Sep 13, 2024

mitchell-as commented Sep 16, 2024

Naatan Sep 19, 2024

Naatan Sep 19, 2024

Naatan Sep 19, 2024

mitchell-as Sep 26, 2024

Naatan Sep 19, 2024

Naatan Sep 19, 2024

mitchell-as commented Sep 26, 2024

mitchell-as commented Sep 26, 2024

mitchell-as commented Sep 26, 2024

Add separate checkout info block to buildscripts. #3483

Add separate checkout info block to buildscripts. #3483

Conversation

mitchell-as commented Sep 9, 2024 • edited by github-actions bot Loading

Choose a reason for hiding this comment

mitchell-as commented Sep 10, 2024

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Sep 12, 2024 • edited Loading

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as commented Sep 26, 2024

mitchell-as commented Sep 26, 2024

mitchell-as commented Sep 26, 2024

mitchell-as commented Sep 9, 2024 •

edited by github-actions bot

Loading

mitchell-as commented Sep 12, 2024 •

edited

Loading